-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Add inline code comments on commits (#4898) #35589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This feature enables users to add inline code comments on individual commits, similar to the existing PR code review functionality. Comments can be added, edited, and deleted on any line of a commit's diff. Changes: - Add service layer functions for commit comment CRUD operations - Add router handlers and API routes for commit comments - Add model queries to find commit comments - Update diff templates to show comment UI on commit pages - Load and display commit comments in the Diff() handler - Reuse existing PR comment templates and JavaScript Implements: #4898
dfbfd56
to
a706a04
Compare
I don't think the comment type |
Instead of reusing CommentTypeCode (which is for PR code comments), introduce a new CommentTypeCommitCode type specifically for inline comments on commits. This provides better separation of concerns and makes the codebase more maintainable. Changes: - Add CommentTypeCommitCode constant and string representation - Update comment type methods to support the new type - Update all commit comment functions to use CommentTypeCommitCode - Handle commit comments separately in updateCommentInfos
Good point! I've updated the implementation to use a separate |
} | ||
|
||
// FindCommitComments finds all code comments for a specific commit | ||
func FindCommitComments(ctx context.Context, repoID int64, commitSHA string) (CommentList, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an index for the query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently there's an index on type
but not on commit_sha
. This function would benefit from an index on commit_sha
(or a composite index on (commit_sha, type)
) since it queries by that field directly. Should I add one in a migration?
} | ||
|
||
// FindCommitLineComments finds code comments for a specific file and line in a commit | ||
func FindCommitLineComments(ctx context.Context, commitSHA, treePath string) (CommentList, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to filter by tree path and line in the memory otherwise, we need a new index for the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated FindCommitLineComments
to filter by tree path in memory instead of in the database query, avoiding the need for a new index on tree_path
. However, both this function and FindCommitComments
would still benefit from an index on commit_sha
since they query by that field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a new composite index repo_id
and commit_sha
and the issue_id should be zero. Because it maybe conflicted with a pull request's commit_sha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the Comment table doesn't have a repo_id
field. To implement this, I would need to:
- Add a
repo_id
column to the comment table (migration) - Create a composite index on
(repo_id, commit_sha)
- Ensure
issue_id = 0
for standalone commit comments - Update
CreateComment
to populaterepo_id
- Update
FindCommitComments
andFindCommitLineComments
to filter byrepo_id
Should I proceed with creating this migration?
Filter by tree path in memory instead of in database query to avoid requiring a new database index. This improves performance by using existing indexes on commit_sha and type.
func FindCommitComments(ctx context.Context, repoID int64, commitSHA string) (CommentList, error) { | ||
comments := make([]*Comment, 0, 10) | ||
return comments, db.GetEngine(ctx). | ||
Where("commit_sha = ?", commitSHA). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repo_id
was missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Currently, commit comments don't have a repo_id
field in the table - they only store commit_sha
. This means we can't filter by repository in the current schema.
We have a few options:
- Add a
repo_id
field to the comment table (requires migration) - Remove the
repoID
parameter since it can't be used - Accept that the same commit_sha across forks would show all comments
What would you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3. Accept that the same commit_sha across forks would show all comments
I don't think this is accepted. So that I think repo_id
is a MUST. Maybe we can have a new table for commit_comment which have a repo_id and a comment_id. So that we don't need to add a new column for comment table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - a separate commit_comment
table would be cleaner. So the structure would be:
New commit_comment
table:
id
(pk)repo_id
(indexed)comment_id
(indexed, foreign key to comment.id)commit_sha
(indexed)- Composite index on
(repo_id, commit_sha)
Then when querying, we'd join with this table to filter by repo. This keeps the comment table unchanged and makes the commit-specific metadata explicit.
Should I proceed with implementing this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds better than before. But I don't know wether it could be accepted by other @go-gitea/maintainers .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - this is a deeper architectural issue. The attachment system expects bindings to issues/releases, so commit comment attachments (without issue_id
) would be incorrectly deleted as orphaned.
Given the complexity here, I'll wait for maintainer consensus on the best approach before proceeding. Please tag me when there's a decision on how to handle this properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@delvh Dude....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm out. Solve it for yourself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good luck!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the misunderstanding. Maybe some messages read too like "vibe coding" and there were a lot of AI PRs from other contributors 🤣 Apologize again.
By the way I am not a native speaker, so please point out if my comment doesn't read good.
By your analysis, I think I can see a potential feasible approach now, although not completely clear yet, share some points:
- The
repo_id
is still necessary, I think only the commit comments from origin repo can be edited or replied. - Adding a new
commit_comment
table seems the only solution . I don't thinkComment.CommitSHA
can be reused since it is use for "CommentTypeCommitRef" and there is no index and it's not suitable to add an index for it DeleteOrphanedAttachments
should be updated to respect "comment_id"- How to "reply" a comment? Maybe
commit_comment.parent_comment_id
?
Summary
This PR implements inline code comments on individual commits, similar to the existing PR code review functionality.
Features
Implementation Details
Comment
model infrastructure (CommitSHA, Line, TreePath already supported)/commit/:sha/comments
for create/update/deletePageIsCommit
is trueTesting
Fixes #4898
/claim #4898